lib.mkOptions: allow arbitrary option metadata#341199
lib.mkOptions: allow arbitrary option metadata#341199hsjobeki wants to merge 8 commits intoNixOS:masterfrom
Conversation
008bb68 to
45b1c5f
Compare
|
I think this is a good direction.
I'm a little concerned with the UX here because the attribute names won't be checked, or the values for that matter. For it to remain extensible, the type checking would have to added as part of Or would could remove the I think we'll also want to propagate the type information from a module evaluation to all the contained types. We already propagate context like the option prefix, so this would have to follow that pattern. I don't know if this makes the evaluation significantly more expensive. I guess this is the tricky part then. |
|
I thought of some freeform I'm not sure what would happen in
In the first In the |
I think it makes more sense to allow any arbitrary meta values to be passed in, since
I think |
|
@roberth What is missing until we can/want to decide if we want to merge this. So should we initially forward the bespoke attributes (description, defaultText, ...) ? Until we actually use them instead of the old ones. Or whats the plan for moving on with this idea? |
It would be up to a few of the many return sub-values of I'm ok to start free-form, if you think it's ok to introduce
I'm not quite certain because I don't have a complete picture for how the migration would go.
So then that means the main code may be ok, but I do have a question. How do you plan to use this field? It doesn't seem that it's propagated to the Could you add a test case showing how you'd like to use this? We have tests in Could you add documentation in |
That makes sense, however I think an additional goal can be to allow module maintainers and/or out-of-tree users to add additional (arbitrary) custom metadata to modules
Sure, it makes sense that well-known attr names would be type-checked where they are used. Such as asserting that
I think that's inevitable, if we want |
|
Okay so I have two motivations for doing this:
My concrete scenario: I have a set of predefined frontend components (input fields, field arrays, ...). I want the author of a module to define options with added information for rendering and appearance. JobWhenToRun = lib.mkOption {
type = lib.types.str;
meta.renderer = ''DateTime"
meta.minDate = "now"
....
}; |
That is not at all the discussion. The discussion is whether they should be checked; we've already settled that the declarations won't be exclusively in Nixpkgs. |
Sounds like we're all on the same page 🙂 I hadn't considered a If that is the eventual goal, perhaps it'd be best to have it in place from the start, to avoid any downstream users extending |
lib.modules.evalOptionValue is still available
1138bb4 to
2eda2ad
Compare
|
Came up with an implementation. Could use a few more tests for corner cases and other things we care about, and docs of course. For now I'm curious about the performance impact and whether anything needs to be added to make this practical. I think inheriting meta attributes into submodules could be nice, but that requires changes to the merge function; same for |
|
Thanks for doing the hard work. I'd like to try to improve the error messages in this case. options.foo = lib.mkOption {
type = types.string;
};
=>
error: The option `_modules.optionMeta.required' was accessed but has no value defined. Try setting the option.This is a little bit confusing because the option is missing an attribute to mkOption. A user would never set I tried various combinations of malformed meta options. Most of them produce confusing error messages. Although they might be technically correct. I am not sure how to fix this, since Maybe we need to fix this in a follow up PR. I thought to make evalModules accept additional error context. That can be printed for giving the error message more meaning. Maybe this could improve the overall error message situation, which is one of the weak points with the module system. |
|
Also what is more performant?
Because patterns are extendable with new arguments without breaking backwards compatibility. |
… prefix
This feels like cheating, but it works out much better for the
error messages:
nix-repl> :p (evalModules { modules = [ {
options.foo = mkOption { };
options._module.optionMeta.wonk = mkOption { };
} ]; }).options.foo.meta
error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option.
|
Nix has optimizations for the positional style, so I would expect I've changed the prefix to contain the option path. Feels like cheating, but it results in much better errors: nix-repl> :p (evalModules { modules = [ {
options.foo = mkOption { };
options._module.optionMeta.wonk = mkOption { };
} ]; }).options.foo.meta
error: The option `options.foo.meta.wonk' was accessed but has no value defined. Try setting the option.Other error improvements may have to go into follow-ups, but I'm curious what they are and whether this solves a large part of it. |
b350a28 to
67ab16a
Compare
d5ee0b5 to
4900b00
Compare
4900b00 to
3afe871
Compare
|
@roberth I thought maybe it makes sense to allow global variables to evalModules. Something like this schematically: |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/zurich-24-15-zhf-hackathon-report/59250/1 |
Description of changes
Allows adding arbitrary metadata to options.
@roberth @infinisil I am not sure if this is a good idea. But I could need it to build a nice user interface around the modules.
Examples
Justification for the examples:
(Especially when trying to build a more high level user interface)
-_* \/^) or just where the name of the option might not make sense for a certain user anymore. I.e.title = "FooBar";would allow to display the option with a more explanatory label without creating an intermediate option, just for the purpose of rendering.I am not sure if we should add those use-cases to the current keywords, or if we should just allow arbitrary names because i am not sure how the above described scenarios would ideally be represented through new keywords.
Alternatives
instead of explicit
meta ? {}passthruinstead of meta, maybe it is more clear.Could produce less thunks but also is less safe towards future extensions of mkOption attrs.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.